Conversation
09bf117 to
664d985
Compare
constantine/ecdsa_secp256k1.nim
Outdated
| message: openArray[byte], | ||
| nonceSampler: NonceSampler = nsRandom) {.libPrefix: prefix_ffi, genCharAPI.} = | ||
| nonceSampler: NonceSampler = nsRandom, | ||
| H: type CryptoHash = sha256) {.libPrefix: prefix_ffi, genCharAPI.} = |
There was a problem hiding this comment.
exportc procedures cannot have a generic type input.
It's fine for the file to be eth_ecdsa_signatures.nim and specialize everything for Ethereum.
There was a problem hiding this comment.
Oh, right! Yeah, let's go with that. It's trivial to add another at some point in the future after all.
| cttEVM_PointNotInSubgroup | ||
| cttEVM_VerificationFailure | ||
| cttEVM_InvalidSignature | ||
| cttEVM_InvalidV # `v` of signature `(r, s, v)` is invalid |
There was a problem hiding this comment.
Is there a point in separating InvalidV from InvalidSignature?
There was a problem hiding this comment.
In practice, I'd say no. Here I was just a bit hasty in mapping the errors from the test vectors to enum elements. Will go with MalformedSignature as below for both.
| return cttEVM_InvalidV | ||
| let v = input[63] | ||
| if v notin [byte 0, 1, 27, 28]: | ||
| return cttEVM_InvalidSignature |
There was a problem hiding this comment.
I would rename MalformedSignature as InvalidSignature is ambiguous whether it's:
- a real signature but not for this public key + message
- something that isn't a signature in the first place
| var point1 {.noinit.}, point2 {.noinit.}: ECJac | ||
| point1.scalarMul(u1, G) # `p₁ = u₁ * G` | ||
| point2.scalarMul(u2, R) # `p₂ = u₂ * R` | ||
| Q.sum(point1, point2) # `Q = p₁ + p₂` |
There was a problem hiding this comment.
This comes up quite often for ECDSA and should be a dedicated proc, unsure about the name yet dualScalarMul, simul, lincomb.
The multiscalarmul_vartime proc has too much overhead for just 2:

but there are the Strauss or the Yao algorithms to accelerate this, especially given that it's a bottleneck for TLS:
There was a problem hiding this comment.
Also the scalarMulEndo routines like this
constantine/constantine/math/elliptic/ec_scalar_mul_vartime.nim
Lines 252 to 331 in 6b65b0e
actually are implemented in 2 steps:
- splitting a scalar in 2 or 4
- and computing a dual or quadruple scalar multiplications efficiently, including in constant-time.
So it should be possible to isolate those internals in a separate PR and redirect the publicly exposed dualScalarMul to use these and minimize extra code that needs to be written.
|
Rebased onto the ECDSA PR branch & made the ECDSA file for secp256k1 specific to Ethereum now. The CI should pass now (aside from the maybe failing Windows test case like in the other where it gets terminated at some point?). edit: or maybe not, yet. 😆 |
|
CI is failing also on Linux, because of an outdated OpenSSL version on Ubuntu 22.04 ships OpenSSL version 3.0.2: while support for Keccak256 was only added in version 3.2 according to this issue comment in July 2024: "Funnily" enough, Keccak is not mentioned anywhere in the actual changelog: According to the OpenSSL docs: version 3.2 is indeed the first to support it (changing to version 3.1 in the docs links leads to a page not found). NOTE: Even Ubuntu 24.04 ships with OpenSSL 3.0.13. Edit: Ubuntu 24.10 is the first version to ship with 3.3. |
constantine.nimble
Outdated
| ("tests/t_ethereum_verkle_ipa_primitives.nim", false), | ||
|
|
||
| # Signatures | ||
| ("tests/ecdsa/t_ecdsa_verify_openssl.nim", false), |
There was a problem hiding this comment.
This can be commented out until Ubuntu 25.04 is available in CI (probably June)
There was a problem hiding this comment.
Are the non LTS releases ever part of the Github Actions CI images?
Done so that future public key recovery can simply call `verifyImpl` to verify public key is found (signImpl changed to match).
ECDSA over secp256k1 commonly uses both SHA256 (e.g. Bitcoin) and Keccak256 (e.g. Ethereum). Other combinations may also exist. We default to SHA256 for the time being.
ECRecover provides the message hash and not the message. We need an
API to pass that directly to the internal ECDSA procedure.
We export the impl `vartime` routine for that purpose. We could
alternatively also import that file using `{.all.}`.
We extend the CttEVMStatus enum by two further elements. One for an invalid signature in ECRecover and another for an invalid `v` value.
Co-authored-by: Mamy Ratsimbazafy <mamy_github@numforge.co>
Co-authored-by: Mamy Ratsimbazafy <mamy_github@numforge.co>
Co-authored-by: Mamy Ratsimbazafy <mamy_github@numforge.co>
Given that we generate a C API from the code, we need to differentiate the function names for the types. The default takes a message and this variant takes a digest (as used in Ethereum's precompile for ECRecover).
|
Rebased, force pushed and taken out the ECDSA test for the time being. |
This builds on top of the ECDSA implementation in PR #490 and adds public key recovery. Based on that implementing the
ECRecoverprecompile is quite straightforward (recover an Ethereum address based on a given message digest and signature).We base our implementation of the precompile on Geth's implementation here:
https://github.com/ethereum/go-ethereum/blob/341647f1865dab437a690dc1424ba71495de2dd8/core/vm/contracts.go#L243-L272
and to a lesser extent appendix F in the Ethereum Yellow Paper.
For the general ECDSA public key recovery API I went with a variable time implementation. To my understanding this is essentially needed regardless, due to the fact that the signing process converts the coordinate of the point
RinFpto a scalar inFr. While unlikely for Secp256k1 (because the subgroup order and the curve prime are both 256 bit integers), if the x coordinate is larger than the subgroup order, we reduce it. This reduction needs to be reversed in the recovery process requiring a loop.In addition we have an
evenYboolean argument, which decides if we recover the public key associated with theRcoordinate with an evenycoordinate or an odd one (ifyis even then-yis odd in a finite field of odd size).(I'll fix the CI failure in the other PR and will rebase this one on top of the other once that is done)